-
Notifications
You must be signed in to change notification settings - Fork 394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
guide: make plots guide #3830
guide: make plots guide #3830
Conversation
Co-authored-by: Restyled.io <[email protected]>
content/docs/sidebar.json
Outdated
"label": "Plots Configuration and Visualization", | ||
"slug": "plots" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Do you think Plots
is enough? What do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "Data Visualization" for now?
Or something else that will let us include params and metrics (in general) in the future too, per #2572 but we can change it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "Data Visualization" for now?
I like to at least use the word "Plots" or it feels like needless obfuscation. Do you think so, or is "Data Visualization" more accurate in some way?
Or something else that will let us include params and metrics (in general) in the future too, per #2572 but we can change it later.
I feel plots may need its own guide since it's a fairly complex subsystem of DVC on its own, but yeah, I guess it's premature to decide either way right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe Visualizing Plots for now?
"label": "Plots Configuration and Visualization", | |
"slug": "plots" | |
"slug": "visualizing-plots" |
Up to you, I just think plain "Plots" is too generic. But we expect guide traffic to come from links in other docs (rather than by browsing the nav) so the name may not be that important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Visualizing plots
works and fits the style of the rest of the other guides, so I'll go with that.
This is mostly a style choice, but I prefer to keep it simple because IMHO it's easier to navigate if the sidebar looks like:
- Experiments
- Pipelines
- Plots
It takes longer for me to find the right section when we add in these descriptive words like:
- Experiments management
- Data pipelines
- Visualizing plots
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Goes back to how exactly we want to organize the user guide. I don't think we can maintain detailed guides for every single feature so specific titles may be more appropriate. Up to you
content/docs/user-guide/plots.md
Outdated
## Example: Offline HTML Template | ||
|
||
The plots generated by `dvc plots` uses Vega-Lite JavaScript libraries, and by | ||
default these load [online resources](https://vega.github.io/vega/usage/#embed). | ||
There may be times when you need to produce plots without Internet access, or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... In the user guide we don't usually have separate examples like these. We embed them into the explanations instead. Think book format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the plot template info is in the new guide now
... ideally we would dissolve the examples into the explanations themselves then, as suggested here.
I'm trying to reorganize first as a quick iteration
OK. Let's try to have a check box somewhere to follow up on this.
content/docs/user-guide/plots.md
Outdated
## Defining plots | ||
|
||
In order to create visualizations, users need to provide the data and | ||
(optionally) configuration that will help customize the plot. DVC provides two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... what I'm suggesting to summarize (heavily probably) and move everything in a good chunk of this section to the dvc.yaml spec, since it's mainly about the schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2... use the guide to show how to setup plots and leave the cmd ref and dvc.yaml pages for lookup purposes
We agree on this @dberenbaum. I just think the Top-level plots explanation can be shorter and leave most schema details for the dvc.yaml "guide".
E.g. phrases like "should be defined ... under plots key" or "Configuration ... should be a dictionary", and the whole Available configuration fields section (as noted). In short, this section is currently a more complete ref than the actual ref.
content/docs/user-guide/plots.md
Outdated
Note that in the case of CSV/TSV metrics files, column names from the table | ||
header (first row) are equivalent to field names. | ||
|
||
### Custom templates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated but should we rename this Custom Vega templates since there's a Custom HTML templates H2 right after this?
Only thing to add to this. I would try to avoid making command references way too short. They should be like a man page still. It has some dry formal description of a behavior, some examples to grasp the idea fast, etc. User guide is definitely higher level. In case of plots, similar to the current index page https://dvc.org/doc/command-reference/plots I think. And can go even a bit higher level to describe things like how to get the data in the first place - |
Agree
Hmmm. Yeah TBH the UG is the most confusing part of our docs conceptually rn, probably because it is both high-level and the most low-level some times (e.g. it includes implementation details). Maybe there's no way to completely separate them if we want a true, book-like "guide". But also the level of effort to keep a complete UG like that (which may overlap a lot with other types of content) is pretty high -- something to consider.
|
To clarify, that's what this PR currently does. However, I understand the concern from @jorgeorpinel that what's there is too low level and more of a reference than a guide. 🤔 ⌛ |
content/docs/user-guide/plots.md
Outdated
`test_logs.csv` against the `epoch`. Note that both files have to have `epoch` | ||
field. | ||
|
||
### Available configuration fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pared Should template
be added here?
I did another pass and made some changes:
|
* guide: add high-level plots guide content * guide: fix plots guide links * Restyled by prettier (#3870) Co-authored-by: Restyled.io <[email protected]> Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <[email protected]>
{ | ||
"slug": "visualizing-plots" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be shortened to just
{ | |
"slug": "visualizing-plots" | |
}, | |
"visualizing-plots" |
p.s. there's an unnecessary \n
in line 167.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPDATE: I'm fixing these in #3903
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorgeorpinel Please don't! I already submitted a PR in #3902
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already submitted a PR in #3902
Oops. My bad
Thanks for all the updates and sorry for late check @dberenbaum . Left some specific suggestions (minor) above... https://dvc.org/doc/user-guide/visualizing-plots and https://dvc.org/doc/user-guide/project-structure/dvcyaml-files#top-level-plot-definitions did end up having all the info they need I think. Of course we can always improve on docs but at least they seem to serve their intended purpose as they are now 👍🏼 Some thoughts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...Relatively minor review of dvc.yaml doc changes (for readability)
The list of `plots` contains one or more user-defined top-level plots (paths | ||
relative to the location of `dvc.yaml`). | ||
|
||
This example makes output `auc.json` viable for visualization, configuring keys | ||
`fpr` and `tpr` as X and Y axis, respectively: | ||
Every plot has to have its own ID. Configuration, if provided, should be a | ||
dictionary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combine intro P
The list of `plots` contains one or more user-defined top-level plots (paths
relative to the location of `dvc.yaml`).
Every plot needs a unique file path as identifier.
Optional configuration can be given as a dictionary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every plot needs a unique file path as identifier.
You can have many different plots all based on the same file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
different plots all based on the same file
I see. This "path vs ID" situation is a bit confusing. Explanations could improve on that I think... ⌛
* guide: edits per #3830 (review) * guide: improvements to Plots * guide: improve Plot spec * ref: improve plots a little and other edits in Plots guides * Update content/docs/user-guide/visualizing-plots.md * guide: list top-level plots before stage * guide: combine jorge and dave edits for plots * guide: consolidate plots guide Co-authored-by: dberenbaum <[email protected]>
This is a quick iteration to address #3818. I made a new guide page since I thought shoving that all into the dvcyaml/project structure would be way too much/messy and hard to find.
Also related: #2572
In review app: https://dvc-org-guide-plots-u9q3eipvdr.herokuapp.com/doc/user-guide/plots